Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: uppercase in path #2170

Merged
merged 7 commits into from
Jul 17, 2023
Merged

Conversation

Jannchie
Copy link
Contributor

πŸ”— Linked issue

close #2162

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Currently, if we construct a path like /zh-CN/foo.md, the corresponding document cannot be found. This is because the path is automatically converted to lowercase. I think the existence of such a path is reasonable and should not be converted. I don't understand why this is done, so in order not to introduce destructive changes, I added a flag named keepUppercase to allow uppercase letters in the path.

I wasn't sure where this flag should be placed, so I tentatively put it under the experimental options.

Resolves #2162

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@netlify
Copy link

netlify bot commented Jul 12, 2023

βœ… Deploy Preview for nuxt-content canceled.

Built without sensitive environment variables

Name Link
πŸ”¨ Latest commit 4d47939
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt-content/deploys/64b53f91461d310008289166

@nobkd
Copy link
Contributor

nobkd commented Jul 12, 2023

That's great.

Can you add this option to the docs too?
I think this is the right place: https://github.com/nuxt/content/blob/main/docs/content/4.api/3.configuration.md

Also, I don't really think, that this is a breaking change. So I think that it does not have to be in the experimental features. What do you think?

@nuxt-studio
Copy link

nuxt-studio bot commented Jul 12, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
content Edit on Studio β†—οΈŽ View Live Preview 4d47939

@nuxt-studio
Copy link

nuxt-studio bot commented Jul 12, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
content Edit on Studio β†—οΈŽ View Live Preview 3608cd5

@Jannchie
Copy link
Contributor Author

Jannchie commented Jul 12, 2023

Also, I don't really think, that this is a breaking change. So I think that it does not have to be in the experimental features. What do you think?

You are right, I moved this option and added the documentation! Thanks for the continued follow up.

src/module.ts Outdated
*
* @default false
*/
keepUppercase: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name may be too vague since we do not understand it's related to path.

Maybe something like respectPathCase, when false, it lower case or respectFilenameCase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rename the name to respectPathCase.

docs/content/4.api/3.configuration.md Outdated Show resolved Hide resolved
@@ -410,3 +410,10 @@ Toggles the document-driven mode.
| `layoutFallbacks` | `string[]` | A list of `globals` key to use to find a layout fallback. |
| `injectPage` | `boolean` | Inject `[...slug].vue` pre-configured page |
| `trailingSlash` | `boolean` | Add a slash at the end of `canonical` and `og:url` |

## `respectPathCase`
Copy link
Contributor

@Barbapapazes Barbapapazes Jul 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could provide an example to help dev to understand types of issues that this option can solve

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect πŸ‘Œ

@Barbapapazes
Copy link
Contributor

Nice PR!

Copy link
Member

@farnabaz farnabaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect πŸ‘
Thanks ❀️

@@ -67,7 +67,8 @@ const isPartial = (path: string): boolean => path.split(/[:/]/).some(part => par
* @returns generated slug
*/
export const generatePath = (path: string, { forceLeadingSlash = true } = {}): string => {
path = path.split('/').map(part => slugify(refineUrlPart(part), { lower: true })).join('/')
const { content } = useRuntimeConfig().public
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid using Nuxt generated helpers (like useRuntimeConfig) in transformers. Transformers and in general the content parser could be able to work outside of Nuxt Context.

There is already several usecases of parser outside of Nuxt.

respectPathCase could be passed in second argumant of this function and transformer itseld

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I've now moved it into the options of the transformer.

@Jannchie Jannchie requested a review from farnabaz July 17, 2023 13:19
@farnabaz farnabaz merged commit 9bd31d6 into nuxt:main Jul 17, 2023
@Jannchie Jannchie deleted the feat/allow-uppercase-in-path branch July 17, 2023 20:20
@farnabaz farnabaz mentioned this pull request Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable use hyphen in path folder
4 participants